Conversation
kripken
left a comment
kripken
left a comment
There was a problem hiding this comment.
Looks like latest Ubuntu LTS (18.04, Bionic) has 8.10. I'm not comfortable going later than that, as it's a reasonably new version, and may be common on CI etc.
In fact I worry other plenty of people may be using previous LTS still, which has 4.2...
ChangeLog.md
Outdated
|
|
||
| Current Trunk | ||
| ------------- | ||
| - Update minimum nodejs version from v4.1.1 for v12.0.0 (Current LTS) |
There was a problem hiding this comment.
nit - no need to capitalize Current, unless there is some language reason I am not aware of?
|
Remember that most emsdk users are not effected here? Does that change anything for you? I'll change this to 8.10.. hopefully that still gives us a lot of modern features. But in general I think supporting old version of node that developers might have installed doesn't need to be a goal of our does it? For non-emsdk users a simple "please upgrade node" message should be very easy to solve no? |
Also require nodejs checks to pass (unless EM_IGNORE_SANITY is set). Fixes: #9332
|
Yeah, asking users to upgrade node isn't so bad, especially if the emsdk installs a proper version. Still, I think supporting latest Ubuntu LTS might be convenient for users. And that version does have modern enough JS for using |
kripken
left a comment
kripken
left a comment
There was a problem hiding this comment.
lgtm aside from that -O1/-O2 issue?
We are reverting the emsdk node version bump so this needs to be reverted too until we can reland.
We reverted the emsdk node version bump, (emscripten-core/emsdk#339, which means this change also needs to be reverted too until we can reland.
We reverted the emsdk node version bump, (emscripten-core/emsdk#339, which means this change also needs to be reverted too until we can reland.
Also require nodejs checks to pass (unless EM_IGNORE_SANITY is set).
…mscripten-core#9345) We reverted the emsdk node version bump, (emscripten-core/emsdk#339, which means this change also needs to be reverted too until we can reland.
2 participants
Also require nodejs checks to pass (unless EM_IGNORE_SANITY is set).
Fixes: #9332